`azurerm_container_app_environment` - add support for Azure Monitor as a log destination #26047

oWretch · 2024-05-22T00:12:17Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Add the logs_destination property to azurerm_container_app_environment to allow specifying Azure Monitor as a log target.

Since changing the log_analytics_workspace_id no longer requires recreating the environment, I have also removed this from the configuration.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_container_app_environment - add support for Azure Monitor as a log destination [azurerm_container_app_environment - add support for Azure Monitor as a log destination #26047]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #25426

tombuildsstuff

hey @oWretch

Thanks for this PR - I've taken a look through and left some comments inline, but I suspect we can infer the value for the logs_destination field from log_analytics_workspace_id having a value, so we might be able to default this to using Azure Monitor unless you specify a Log Analytics Workspace ID, to remove the extra field/hard-coded values - WDYT?

Thanks!

tombuildsstuff · 2024-05-22T10:54:18Z

internal/services/containerapps/container_app_environment_resource.go

+		"logs_destination": {
+			Type:         pluginsdk.TypeString,
+			Optional:     true,
+			Computed:     true,
+			ValidateFunc: validation.StringInSlice([]string{"log-analytics", "azure-monitor"}, false),
+			Description:  "The destination for the application logs. Possible values are `log-analytics` or `azure-monitor`.",
+		},


Two things here:

Rather than making this Computed, which'd mask any default value for this field - can we default this here until 4.0 (at which point we can make this field required).

Whilst I can see that the Description field is a string value, do you have a canonical source for the casing of the values log-analytics and azure-monitor - is that what the API is returning/defined somewhere? Typically we source these from hashicorp/go-azure-sdk, but since this is a String field we don't have a backing constant for that, so it'd be good to confirm the casing (and file an upstream issue to fix this, once we have that available)

Suggested change

"logs_destination": {

Type: pluginsdk.TypeString,

Optional: true,

Computed: true,

ValidateFunc: validation.StringInSlice([]string{"log-analytics", "azure-monitor"}, false),

Description: "The destination for the application logs. Possible values are `log-analytics` or `azure-monitor`.",

},

// TODO: make this Required in 4.0

"logs_destination": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringInSlice([]string{"log-analytics", "azure-monitor"}, false),

Description: "The destination for the application logs. Possible values are `log-analytics` or `azure-monitor`.",

},

Alternatively we could infer the value for this field based on log_analytics_workspace_id having a value, which is probably better here, since there's only 2 sources?

The reference to the strings comes from the REST API documentation, though it is worth noting that the documentation implies that none is a valid value, when the actually mean that the object is empty. This is more clear in the API error message which doesn't quote none.

Which also means we don't want to make the property required, as it is perfectly valid to not configure the log destinations at all.

tombuildsstuff · 2024-05-22T10:57:21Z

internal/services/containerapps/container_app_environment_resource.go

+					if appLogsConfig := props.AppLogsConfiguration; appLogsConfig != nil {
+						state.LogsDestination = *appLogsConfig.Destination
+					}


if an existing Container App Environment (which doesn't set LogsDestination) is imported into Terraform/queried via the API - is the Destination field null/have a default value?

Depends on the existing setting. A default creation, without specifying a LAW has the LogsDestination as an empty object in the JSON.

internal/services/containerapps/container_app_environment_resource.go

oWretch · 2024-05-22T21:21:28Z

hey @oWretch

Thanks for this PR - I've taken a look through and left some comments inline, but I suspect we can infer the value for the logs_destination field from log_analytics_workspace_id having a value, so we might be able to default this to using Azure Monitor unless you specify a Log Analytics Workspace ID, to remove the extra field/hard-coded values - WDYT?

Thanks!

@tombuildsstuff Thanks for the review. I debated how to expose the Azure Monitor setting. You are correct that we can infer the setting based on log_analytics_workspace_id as this is what the provider was already doing. Microsoft have added a second target which uses the standard diagnostic settings for configuration, which then allows for multiple target workspaces.

A second option I considered was to add a property such as enable_diagnostic_settings instead of logs_destination and make it conflict with log_analytics_workspace_id, and then configure the logging settings in light of this pair of properties.

Ultimately I decided to expose the underlying setting to first provide a simple way to extend the configuration if Microsoft add a new method in future, and second to conform to the way these settings appear in the Portal. The third is that there are actually three options - the default is that there is no log configuration, so I wanted to expose that option also.

I'm happy to go with the above suggestion (though the property name probably needs some discussion).

katbyte

Tests are failing with

------- Stdout: -------
=== RUN   TestAccContainerAppEnvironmentCertificate_basicUpdateTags
=== PAUSE TestAccContainerAppEnvironmentCertificate_basicUpdateTags
=== CONT  TestAccContainerAppEnvironmentCertificate_basicUpdateTags
    testcase.go:113: Step 1/4 error: Error running apply: exit status 1
        
        Error: `log_analytics_workspace_id` can only be set when `logs_destination` is set to `log-analytics`
        
          with azurerm_container_app_environment.test,
          on terraform_plugin_test.tf line 39, in resource "azurerm_container_app_environment" "test":
          39: resource "azurerm_container_app_environment" "test" {
        
--- FAIL: TestAccContainerAppEnvironmentCertificate_basicUpdateTags (97.37s)
FAIL

Dilergore · 2024-07-26T06:42:46Z

hi,

Is there a plan to merge this?

Ronbabious · 2024-09-24T13:30:12Z

What is the plan for this PR. This feature would be great to have.

jackofallops

Hi @oWretch - Thanks for this, and apologies it's taken a while to get back to it, we've had some team changes and this one slipped through the cracks.
I've left 2 comments below, once they're addressed I think this will be good to run the CI tests and if they're good, we should be able to get this merged.

Thanks

jackofallops · 2024-12-06T04:58:40Z

internal/services/containerapps/container_app_environment_resource.go

+				return fmt.Errorf("`log_analytics_workspace_id` must be set when `logs_destination` is set to `log-analytics`")
+			}
+
+			if env.LogAnalyticsWorkspaceId != "" && env.LogsDestination != "" && env.LogsDestination != "log-analytics" {


I think this is simpler as

Suggested change

if env.LogAnalyticsWorkspaceId != "" && env.LogsDestination != "" && env.LogsDestination != "log-analytics" {

if env.LogsDestination != "log-analytics" && env.LogAnalyticsWorkspaceId != "" {

…s a log destination

jackofallops · 2024-12-11T09:40:49Z

Hi @oWretch - I hope you don't mind, but we have an internal support request for this, so I'm going to push some changes and fix the merge conflicts to get this in.

fix crash in read fixup customizeDiff logic add consts to remove magic strings

oWretch · 2024-12-11T18:52:25Z

Hi @oWretch - I hope you don't mind, but we have an internal support request for this, so I'm going to push some changes and fix the merge conflicts to get this in.

I don't mind at all 🙂 This week has been crazy so I didn't have the time to get back to it - I'm happy to see it progress.

catriona-m · 2024-12-19T13:51:28Z

Hi @oWretch and @jackofallops - I fixed the linting on this to see if we could get this merged, but unfortunately there are test failures that would need to be investigated first:

------- Stdout: -------
=== RUN   TestAccContainerAppEnvironment_updateWorkloadProfile
=== PAUSE TestAccContainerAppEnvironment_updateWorkloadProfile
=== CONT  TestAccContainerAppEnvironment_updateWorkloadProfile
    testcase.go:173: Step 7/12 error: Error running pre-apply plan: exit status 1
        Error: `log_analytics_workspace_id` must be set when `logs_destination` is set to `log-analytics`
          with azurerm_container_app_environment.test,
          on terraform_plugin_test.tf line 78, in resource "azurerm_container_app_environment" "test":
          78: resource "azurerm_container_app_environment" "test" {
--- FAIL: TestAccContainerAppEnvironment_updateWorkloadProfile (2323.99s)
FAIL

github-actions bot added documentation service/container-apps size/S labels May 22, 2024

oWretch changed the title ~~azurerm_container_app_envrionment - add support for Azure Monitor as a log destination~~ azurerm_container_app_environment - add support for Azure Monitor as a log destination May 22, 2024

github-actions bot added waiting-response size/M and removed waiting-response size/S labels May 22, 2024

tombuildsstuff suggested changes May 22, 2024

View reviewed changes

github-actions bot added the waiting-response label May 22, 2024

github-actions bot removed the waiting-response label May 22, 2024

oWretch requested a review from tombuildsstuff May 22, 2024 21:36

oWretch mentioned this pull request May 27, 2024

azurerm_container_app_job Rename plural properties to singular for consistency with other Container App resources #26063

Merged

13 tasks

katbyte requested changes Jun 4, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Jun 4, 2024

oWretch requested a review from katbyte June 4, 2024 07:46

rcskosir added the enhancement label Oct 14, 2024

katbyte requested a review from a team as a code owner November 14, 2024 00:08

jackofallops requested changes Dec 6, 2024

View reviewed changes

jackofallops added the waiting-response label Dec 11, 2024

oWretch added 3 commits December 11, 2024 10:08

azurerm_container_app_envrionment - add support for Azure Monitor a…

dd6e374

…s a log destination

Fix code formatting

f9bb524

Fix validation error for legacy

6a93346

jackofallops self-assigned this Dec 11, 2024

add tsts for customizeDiff

6a9693c

fix crash in read fixup customizeDiff logic add consts to remove magic strings

jackofallops force-pushed the f/container-apps-log-settings branch from f02f256 to 6a9693c Compare December 11, 2024 10:38

github-actions bot added size/L waiting-response and removed waiting-response size/M labels Dec 11, 2024

github-actions bot removed the waiting-response label Dec 11, 2024

remove space

20c7075

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_container_app_environment` - add support for Azure Monitor as a log destination #26047

`azurerm_container_app_environment` - add support for Azure Monitor as a log destination #26047

oWretch commented May 22, 2024 •

edited by rcskosir

Loading

tombuildsstuff left a comment

tombuildsstuff May 22, 2024

tombuildsstuff May 22, 2024

oWretch May 22, 2024 •

edited

Loading

tombuildsstuff May 22, 2024

oWretch May 22, 2024

oWretch commented May 22, 2024 •

edited

Loading

katbyte left a comment

Dilergore commented Jul 26, 2024

Ronbabious commented Sep 24, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Dec 6, 2024

jackofallops commented Dec 11, 2024

oWretch commented Dec 11, 2024

catriona-m commented Dec 19, 2024

	if env.LogAnalyticsWorkspaceId != "" && env.LogsDestination != "" && env.LogsDestination != "log-analytics" {
	if env.LogsDestination != "log-analytics" && env.LogAnalyticsWorkspaceId != "" {

azurerm_container_app_environment - add support for Azure Monitor as a log destination #26047

Are you sure you want to change the base?

azurerm_container_app_environment - add support for Azure Monitor as a log destination #26047

Conversation

oWretch commented May 22, 2024 • edited by rcskosir Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 22, 2024

Choose a reason for hiding this comment

tombuildsstuff May 22, 2024

Choose a reason for hiding this comment

oWretch May 22, 2024 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff May 22, 2024

Choose a reason for hiding this comment

oWretch May 22, 2024

Choose a reason for hiding this comment

oWretch commented May 22, 2024 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

Dilergore commented Jul 26, 2024

Ronbabious commented Sep 24, 2024 • edited Loading

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Dec 6, 2024

Choose a reason for hiding this comment

jackofallops commented Dec 11, 2024

oWretch commented Dec 11, 2024

catriona-m commented Dec 19, 2024

`azurerm_container_app_environment` - add support for Azure Monitor as a log destination #26047

`azurerm_container_app_environment` - add support for Azure Monitor as a log destination #26047

oWretch commented May 22, 2024 •

edited by rcskosir

Loading

oWretch May 22, 2024 •

edited

Loading

oWretch commented May 22, 2024 •

edited

Loading

Ronbabious commented Sep 24, 2024 •

edited

Loading